Convert FixedOffset::{east, west} to return Result
#1468
Conversation
pitdicker
force-pushed
the
convert_fixed_offset
branch
from
837d1ab to
68d9e3e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0.5.x #1468 +/- ##
==========================================
+ Coverage 94.33% 94.34% +0.01%
==========================================
Files 37 37
Lines 17004 17009 +5
==========================================
+ Hits 16040 16047 +7
+ Misses 964 962 -2 ☔ View full report in Codecov by Sentry. |
|
The spelling of the existing docs doesn't seems entirely correct English. I'll adjust that. |
| let fixed_offset = FixedOffset::east(secs) | ||
| .expect("Could not generate a valid chrono::FixedOffset. It looks like implementation of Arbitrary for FixedOffset is erroneous."); | ||
| Ok(fixed_offset) | ||
| Ok(FixedOffset::east(secs).unwrap()) |
There was a problem hiding this comment.
Could the unwrapping be prevented by mapping the error? Untested: FixedOffset::east(secs).or(Err(arbitrary::Error))
There was a problem hiding this comment.
The range we supply to arbitrary::Unstructured::int_in_range is exactly equal to the range FixedOffset supports, and I don't see that changing. This should simply never fail, so I even considered the expect to be overkill.
| } else { | ||
| None | ||
| pub const fn east(secs: i32) -> Result<FixedOffset, Error> { | ||
| match -86_400 < secs && secs < 86_400 { |
There was a problem hiding this comment.
While I appreciate the conversion to match, in the future I would appreciate it more in a separate commit. 👍
The easiest methods to convert yet!
Both
Error::InvalidArgumentandError::OutOfRangecould be reasonable return types.The real-world limit of offsets is currently 14 hours, which puts area's using it 2 hours to the other side of the international date line.
The type limit is generous and somewhat arbitrarily limited to be less than 24 hours. At some point it turned out limiting the offset to less than a day keeps the surrounding code saner.
Because the limit is artificial and specific to chrono I went with
Error::OutOfRange.cc @Zomtir.